I have tested this only on fixes/33, but no changes seem to have been made in that area between that and master. The rebase went cleanly without conflicts.

Could this be related to #773 ?

Could this be related to #773 ?

I don't believe so. It's a shame that #773 is specific to master: I've just been trawling through what must be code relevant to #773, but only the fixes/33 version (I don't run master and haven't found an easy way to test it). I will take a look at the fixes/33 vs master diffs and see if any changes stand out within the code I've recently had to understand.

#773 seems to have started after a bunch of changes that were made this year, so it won't apply to fixes/33.

After fixing #773, I tested this (without the pull request). Pressing T while forced subtitles are in effect displays a message that the forced subtitles are on or off but does not have any effect on their display. This is clearly a bug.

The T switches between the menu items "Enable subtitles" and "Disable subtitles" while ignoring the menu item "Disable Forced Subtitles"

IMHO, the T (toggle) should turn off the subtitles and turn them on again. That is useful. If you cannot hear something you turn them on, then once you are hearing the dialog better you turn them off. It does not make sense for T to switch between two sets of subtitles. What is the use case here? Two people who speak different languages fighting over the remote?

Yes I agree: you want to be able to turn on subtitles when you mishear something. That's the great thing about mythfrontend having a simple toggle for on and off. But here is my thinking in chosing the behaviour of this new code. You want to be able to toggle the subtitles on and off ALL THE WAY THROUGH the video, but the forced ones are useless for this because usually they come into play at just a few points in time - when people are speaking in a language different to the video's default. That is almost always the case with forced subtitles: they are (in a way) off most of the time even when enabled because there just isn't any text in a forced subtitle track over most of a video.

With my changes, you still see the forced subtitles without explicitly enabling (no change there), but you also still get to toggle on a subtitle track that has text for every word spoken in the video, so a feature useful thoughout the video. It makes videos with forced tracks behave very much like videos without forced tracks - so more conistent.

If we updated this to toggle off and on the foced track (when present) then it would be a fairly useless feature: you don't usually need to turn off a forced track because it's usually not present for most of the video. Also, as is the case now, the presence of a forced track would lose you the ability to toggle on a entire-video useful subtitle.

At least give it a try. :-)

And, if you were worried that the user might want to disable all subtitles including the forced track, there is already a separate command for disabling the forced track.

I will wait a few days for the subtitle bug fix to settle down, then test this. If it works, I will ask for comments on the mailing list, in case anybody has a problem with it. If all is OK I will push it then.

Thank you Peter

I am holding back on this because of a long chain of emails about it. There seems to be some controversy.

Yes, makes sense. Controversy and possibly confusion. At the very least it needs establishing that it is working correctly on master.

What are your thoughts on where we should go with this? There haven't been many interested in discussing it. I've had some back and forth with Angela Schmid, which didn't go well, mainly - I believe - because I'd made a mistake resolving the conflicts with your recent changes (hopefully corrected now). In my last post I found a better way of describing the feature, but Angela didn't reply. Since then, both David Hampton and Paul Harrison have chipped in with support for the change (although, I don't get the impression they tested it).

Please confirm that this is the final change that this pull request provides (from your email):

• If a video has no forced track then the player starts in the Off state. TOGGLECC toggles between Off and On.
• If a video does have a forced track then the player starts in the Almost Off state. TOGGLECC toggles between Almost Off and On. If you want no subtitles at all, not even forced then you have to use the menu to and select "Disable forced subtitles", whereupon TOGGLECC will then toggle between Off and On.

If this is correct, I will test in on master and if all works I will commit it.

Thank you Peter.

Yes, the pull request has the latest versions of my commits, and that's the intended behaviour.

@bennettpeter
In my eyes this pull request on master, does not do anything other than what already is implemented on master.

What you wrote is the current behavior on master, with or without the pull request, and differs from what Paul intended with this pull request and it's behavior on v33.

When you think I am wrong, please perform some scenarios once again, with and without the pull request, and explain the difference.

The last week I didn't had time to respond on the current discussion in the mailing list, I will try to do this latest Sunday.

I am concerned, that at one point, with future changes, the behavior of TOGGLECC changes.

If playing a video without a forced subtitle, there is no difference with this pull request.

Current master playing a video with forced foreign subtitles:

• Foreign Subtitles are showing.
• Press T
• Message says forced subtitles are on and the foreign subtitles are still showing.
• Press T again
• Message says forced subtitles are off and the foreign subtitles are still showing.
• With menu select English subtitles
• Message says Englist subtitles are on, and English subtitles show
• Press T
• Message says Englist subtitles are off, and subtitles stop

Current master with pull request playing a video with forced foreign subtitles:

• Foreign Subtitles are showing.
• Press T
• Message says Englist subtitles are on, the foreign subtitles stop, and English subtitles show
• Press T again
• Message says Englist subtitles are off, the English subtitles stop, and foreign subtitles show
• With menu select English subtitles
• Message says English subtitles are on, and English subtitles show
• Press T
• Message says English subtitles are off, and foreign subtitles show

With your tests, you had the global subtitle setting OFF. The behavior is different when ON.

Current master with pull request playing a video with forced foreign subtitles:

• With global setting OFF, it is as you wrote.

From the start the English subtitle is selected in the menu.

When you selected the subtitle via the menu, you have just pressed the already selected English subtitle.
I use NEXTSUBTITLE, where the next subtitle after the English subtitle is chosen, and need to to loop through, to get the English subtitle.

About using the TOGGLECC (definition: toggle subtitle ON/OFF), as with global setting OFF, their is way too much going on:
first T: nothing happens
next T: jumps to subtitle
last T: all subtitles off
what happens with further T's, what is expected ON/OFF or switch forced/non-forced, at which situation?
My concern, too complicated, and users might not get an "expected" behavior when pressing T.

• With global setting ON (which is my preferred setting):

The Foreign Subtitles (forced) is selected in the menu.
Pressing T: shows "forced off", but forced is shown.
Pressing T: shows "forced on", forced is shown.
Selecting a subtitle (menu or NEXTSUBTITLE), on and off work correct
Selecting a forced, on and off: always show

Disabling forced, only works when selecting both "Disable Subtitle" and "Disable Forced Subtitle" from the menu.
A menu misbehavior also exists on master.

• My focus is on global setting ON, I can live that forced are always shown, just want on/off for the subtitle with TOGGLECC to keep working as is. It is similar how master behaves when NEXTSUBTITLE/TOGGLECC and no menu options are used.

• In my comments in the mailing list, I made suggestions to make the subtitle usage easier and fix some issues, unfortunately nobody commented, to come to a consensus/usage/expected behavior.

• Not mentioned: expected would be, that when NEXTSUBTITLE is used, the next subtitle is selected and ON. Currently, it depends on an earlier ON or OFF and it does not show the current state.

* With global setting ON (which is my preferred setting):

The Foreign Subtitles (forced) is selected in the menu.
Pressing T: shows "forced off", but forced is shown.
Pressing T: shows "forced on", forced is shown.

That's the old broken behaviour. I made a mistake in conflict resolution when moving this from fixes/33 to master. I've pushed corrections since our initial discussions. If you wish to test this, you need to pull the new version. All these recent discussions relate to the fixed version.

* In my comments in the mailing list, I made suggestions to make the subtitle usage easier and fix some issues, unfortunately nobody commented, to come to a consensus/usage/expected behavior.

There have indirectly been some discussions of your proposal. I posted to mythtv-users describing 3 options. Option 2 was my attempt to describe your suggestion. Both David Hampton and Paul Harrison preferred Option 3 which is what this change achieves. My description may be wrong but, if so, it would be useful if you explained in what way.

Actually, there could be an unintended dependence on the global setting. Let me check that.

My suggestions do not have directly anything to do with your pull request.

Instead of this, sorry for the wording, horrible intruding behavior of TOGGLECC, which makes it a very unexpected experience. It would also have been nice, when you would have reacted on my concern that it is too complicated. In my mailing list comments, I already wrote about unexpected behavior when further toggling and changing the subtitles (also in combination with the menu).

I am just hoping the TOGGLECC behavior does not change, and commented several times to use another key. The pull request still uses TOGGLECC, so my concern is still valid.

Actually, there could be an unintended dependence on the global setting. Let me check that.

When we only look on the global setting ON, nothing should need to change (in the sense of this pull request):

One of my suggestions was to have an option to set the non-forced subtitle as default, this would make the pull request superfluous. Unfortunately, nobody commented on the two possible ways to solve.
What you wanted to implement with this pull request, was to have an option to switch to the non-forced subtitle. Having an option, what just does this, without any interaction, is so much simpler. We have the global option, when ON, just set to non-forced. In my comments I explained that non-forced also contains all forced subtitles, so nothing gets lost.
I hope my suggestion gets considered.

What I noticed yesterday was, that you have forced-pushed your changes, maybe you did a rebase.
When fetching it says, it received forced-push-ed commits, so I already hoped that I would get the latest changes.
As the git log showed the old commit dates, I made a diff of the old and the new fetched data, and only saw a styling change in the third commit.

I cloned your repo and got the following:

commit 17d0137904e7dc1a4ddf79b2fa64a0b729020595 (HEAD -> improve-forced-subtitle-handling, origin/improve-forced-subtitle-handling)
Author: Paul Gardiner <mythtv@glidos.net>
Date:   Fri Dec 1 19:59:39 2023 +0000

    Avoid displaying two AV subtitle tracks simultaneously

    Recent changes to the handling of forced AV subtitle tracks require that
    two tracks may be considered for processing simultaneously. This happens
    if there is a forced track and a user enabled one. This commit ensures
    that the user enabled one inhibits the forced one.

commit 36bd127832327c86e0f2131d3f877d0358e1a2cf
Author: Paul Gardiner <mythtv@glidos.net>
Date:   Tue Nov 28 15:55:07 2023 +0000

    Handle forced AV subtitles without needing a forced track to be selected

    This improves the behaviour of mythfrontend in its automatic handling of
    forced subtitle tracks. The previous behaviour relied on automatically
    selecting the forced track as a default for the user's preference. The new
    behaviour attempts to find a non-forced track for the user's preference and
    changes the subtitle toggle feature to be between forced and non-forced,
    rather than on and off.

commit 20027e2744c462988bc220b0a21dfd4c5c024330
Author: Paul Gardiner <mythtv@glidos.net>
Date:   Tue Nov 28 12:12:46 2023 +0000

    Factor out the best track calculation for selecting the subtitle track

    Also generalise the algorithm to allow forced tracks to be disfavored.
    This is work towards improving the handling of forced tracks.

The commit dates differ from the github commit dates.
I have seen issues in the past with forced-push at Github, maybe that happened also with yours (it was late yesterday and I haven't investigated further).

Your description of what I'm attempting to achieve is not what I'm attempting to achieve. In a way, I want the same as you, I believe: to fix some strange behaviour. Here is the strange behaviour (referring to mythtv without my changes):

I play videos that have subtitles, and mishear something in the audio. I know that these videos have subtitles so I hit Left Arrow a few times and then T to replay a bit of the video with subtitles on. That sometimes works and sometimes doesn't. The reason that sometimes doesn't work is because the video has a forced track and it's been selected by mythtv as the default. The part of the video where I misheared something had no text in the forced track, so I see no subtitle text. I hate this. I get the impression you hate this too.

The difference in our solutions, I believe, is that you wish to fix it by removing the automatic forced track feature entirely. I want to fix it while maintaining that feature. I very much like getting forced tracks automatically. I just don't want to get them at the expense of easy one key access to the main tracks.

Does that help at least in understanding what I wish to achieve?

In this pull request:

There is a bug when "Always Display Closed Captioning or Subtitles" is selected. Playback starts with English subtitles. Pressing T disables them but does not enable the forced subtitle.

There is another bug which shows whether or not the "Always Display" is selected. Using Menu->Subtitles->Select Subtitle->xxx (forced). causes the forced subtitle to be turned off, while reporting it is on. Pressing T turns the subtitle on while reporting it is off and pressing it again turns the subtitle off while reporting it is on.

In this pull request:

There is a bug when "Always Display Closed Captioning or Subtitles" is selected.
Playback starts with English subtitles. Pressing T disables them but does not enable the forced subtitle.

Yes, I've just noticed that too. I can see the problem. There are some functions regarding forced subtitles that are now unnecessary, but which I'd left in. I also don't like the way that I'm storing one selected forced subtitle rather than one per subtitle type. I think I should have made it more consistent with the storing of the main selected subtitle of each type.

There is another bug which shows whether or not the "Always Display" is selected.
Using Menu->Subtitles->Select Subtitle->xxx (forced). causes the forced subtitle to be turned off, while reporting it is on.
Pressing T turns the subtitle on while reporting it is off and pressing it again turns the subtitle off while reporting it is on.

Yeah, I was sort of aware of something like that, although I thought it was that - while the reporting toggled between on and off, the forced track remains active. It's a strange edge case because the selected and forced track are the same so you are toggling between a track and itself. I'm not sure what best to do about it. At least it's not a regression because its mythtv's current default behaviour when a forced track is present. It's aspect I haven't managed to fix so far.

Yeah, I was sort of aware of something like that, although I thought it was that - while the reporting toggled between on and off, the forced track remains active. It's a strange edge case because the selected and forced track are the same so you are toggling between a track and itself. I'm not sure what best to do about it. At least it's not a regression because its mythtv's current default behaviour when a forced track is present. It's aspect I haven't managed to fix so far.

Not quite the same as current. I retested current master, and if I select the forced subtitle then use T it reports that it is on or off while all the time continuing to display it, regardless of whether it was reported as on or off.

Yeah, I was sort of aware of something like that, although I thought it was that - while the reporting toggled between on and off, the forced track remains active. It's a strange edge case because the selected and forced track are the same so you are toggling between a track and itself. I'm not sure what best to do about it. At least it's not a regression because its mythtv's current default behaviour when a forced track is present. It's aspect I haven't managed to fix so far.

Not quite the same as current. I retested current master, and if I select the forced subtitle then use T it reports that it is on or off while all the time continuing to display it, regardless of whether it was reported as on or off.

I thought it still did that with my changes - although probably only if the global setting is off.

The difference between the two of us is:

* You have global subtitles OFF, show forced and occasional switch to non-forced and immediately back again. In this case you do not need an on/off toggle, as forced are not many, the chance it covers something, which the user wants to disable, are neglectable.

Yes. That's exactly right. We do seem to have a common understanding of what I was intending and why.

* I have global subtitles ON, I would rather have non-forced default on,

Yes, and that would be how it would work with my changes (if I could get them right, that is :-) ). Possibly the current version does do that

I need TOGGLECC to switch on and off.

I'd dispute that you need that. If the toggle switched to forced rather than off, you'd likely not notice because the chances you'd still see subtitles are negligible. On the odd occasion that subtitles remained visible even though you've switched them off, since they come from the forced track, they'd likely be something you wouldn't understand from the audio. It would be a benefit that you haven't been able to turn them off. I see it as just honoring the meaning of the word "forced".

On the other hand, I do see that the change isn't a clear win if you are a user that has the global setting on because the very annoying behaviour I'm fixing has never affected you.

Looking at these two use-cases, the current pull request allows both. When others have another use-case, we still have the ability with a global option to differentiate.

When this is the ultimate implementation of your pull, than it is OK for me.

Yes, by chance, I think the current version gives my intended behaviour when the global setting is off, and yours when it is on. Possibly we could go with that, but - as Peter pointed out - really it's a bug. I would expect the global setting On to behave just like having the setting Off, but hitting TOGGLECC immediately the video starts.

With your use-case and the current pull request: There are several conditions (global setting, having a forced or not, having a non-forced or not, and have you ever pressed T before or chosed from the menu), ending in different outcomes. On each press the outcomes become more unpredictable. I wouldn't go that far to also have a full off state, just let the forced show, like that you are only flipping between the two, the presses become more predictable. Too many states with a single key will confuse users. (I complaint about the broken menu states with global ON, this would otherwise become similar).

I disagree. There is some confusion due to the remaining bugs but, other than that, I think it's just a case of honoring the meaning of the word "forced" and not confusing.

I have a personal patch that disables jump keys in playback (have to exit to use jump keys), i have some 8 keys which I can set, that's why I would rather suggest to create a different key for the flipping.

I made several suggestions, I would liked some comments about how people would like the "subtitle" experience. I still hope some may comment.

Yes, fair enough, but I just haven't the interest to expand this into a more general rework of the subtitle system. The whole thing needs rewriting and it looks a big job. My take on this was that I had a fix for a really annoying behaviour. I've been using it in my own releases and package building. It's working well for me. Maybe it would be useful to others. I've only today become aware that the really annoying behaviour isn't apparent when the global setting is on.

And although I have some sympathy for Klaas's view, I have no appetite for writing wiki pages.

I've just pushed a new version with the bugs fixed.

• The new behaviour now applies with either Always Display Subtitles on or off.

• If the user explicitly selects a forced track, then TOGGLECC will toggle it on and off, rather than just reporting it is doing so.

Looking good, thanks for making a non-forced default. Also the menu is looking better.

The following issues I found:

• When selecting a subtitle in another language and pressing TOGGLECC, the forced stays in the starting language, it would be nice it is re-evaluated.
• When selecting a forced subtitle, the two menu "enable/disable non-forced/forced subtitles" are incorrect/confusing, as you are treating it as a non-forced subtitle.
  • After a select, do you evaluate the type of the subtitle?
  • Are you using the two toggles to control behavior in your logic? Or is it, that you have your logic and are setting the values for these toggles?
  • As most is around TOGGLECC, would it make sense to reduce to only have one menu toggle?

A wish:
Currently NEXTSUBTITLE just changes the "pointer" to the new subtitle, it would be nice it sets the state explicitly to ON.
Maybe the OSD message needs to be changed to explicitly show the state.

Compile give the following warning:

captions/subtitlereader.cpp: In member function ‘bool SubtitleReader::AddAVSubtitle(AVSubtitle&, bool, bool, bool, bool)’:
captions/subtitlereader.cpp:99:32: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
   99 |     if (m_textSubtitlesEnabled && !isExternal || m_avSubtitlesEnabled && isExternal)
      |         ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~

Looking good, thanks for making a non-forced default. Also the menu is looking better.

That's great that you are finding this to be working for you.

The following issues I found:

* When selecting a subtitle in another language and pressing TOGGLECC, the forced stays in the starting language, it would be nice it is re-evaluated.

I too had wondered about that. It could be wasted work as I've never seen a video with more than one forced track in the wild, but it does make logical sense. I'll take a look how difficult it is to do.

* When selecting a forced subtitle, the two menu "enable/disable non-forced/forced subtitles" are incorrect/confusing, as you are treating it as a non-forced subtitle.
  
  * After a select, do you evaluate the type of the subtitle?
  * Are you using the two toggles to control behavior in your logic? Or is it, that you have your logic and are setting the values for these toggles?
  * As most is around TOGGLECC, would it make sense to reduce to only have one menu toggle?

Yes, I see what you mean. That shouldn't be hard to fix.

A wish: Currently NEXTSUBTITLE just changes the "pointer" to the new subtitle, it would be nice it sets the state explicitly to ON. Maybe the OSD message needs to be changed to explicitly show the state.

I think that would need discussion amongst the devs. There could be users of this interface which rely on its current behaviour. Does mythfrontend provide a way to map a key to a sequence of commands? I can't remember. If so that might be something you could use to get the behaviour you want.

Compile give the following warning:

captions/subtitlereader.cpp: In member function ‘bool SubtitleReader::AddAVSubtitle(AVSubtitle&, bool, bool, bool, bool)’:
captions/subtitlereader.cpp:99:32: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]
   99 |     if (m_textSubtitlesEnabled && !isExternal || m_avSubtitlesEnabled && isExternal)
      |         ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~

Noted.

One more commit added: as angelaschmid suggested, on changing the selection of the main subtitle, an attempt is made to update the corresponding forced track to the same language.

I didn't do anything about inhibiting the Disable/Enable Forced Subtitle menu option when the main selection happens to be a forced track. Although it has no immediate effect in that case, someone might want to alter the option ready for a later change of selected main subtitle. I also noticed that mythfrontend displays this option even in the case that none of the available subtitles are forced. We can always come back to this at a later date.

I didn't fix the warning either because it's not caused by the changes in this pull request, as far as I can tell.

I am about to leave for holidays, I will have a look on Jan 8th.
Have you added a change for the compile error?

I am about to leave for holidays, I will have a look on Jan 8th. Have you added a change for the compile error?

Someone else looks to have fixed that compile error.

The commit "Attempt to match forced track language to that of the selected main track" works.

With the pull request

• Current master has issues with the menu states, the pull request does not fix them, even introduces conditions where the menu klicks (multiple klicks) do not change expected behavior any more.
• Having multi language subtitles using NEXTSUBTITLE to select a forced subtitle, does not show the forced subtitle (this does not happen when selecting the first language or default language forced subtitle).
• The toggle does not change the currently selected forced or non-forced subtitle in the menu, which makes menu interactions ambiguous.
  Upon the toggle to disable a subtitle, the OSD can show "subtitle: off", but should show the forced subtitle selected in the menu and vice-versa.

* When selecting a forced subtitle, the two menu "enable/disable non-forced/forced subtitles" are incorrect/confusing, as you are treating it as a non-forced subtitle.
  
  * After a select, do you evaluate the type of the subtitle?
  * Are you using the two toggles to control behavior in your logic? Or is it, that you have your logic and are setting the values for these toggles?
  * As most is around TOGGLECC, would it make sense to reduce to only have one menu toggle?

Yes, I see what you mean. That shouldn't be hard to fix.

In the mailing list I proposed to simplify things (e.g. reducing to have one menu subtitle toggle on/off, unfortunately nobody commented). I think the two on/off states and the toggle states collide and are difficult to handle with menu and key events, and expose them unambiguous back to the menu.

Do you want to fix this in this pull request?

Would this also be your expected behavior?

The commit "Attempt to match forced track language to that of the selected main track" works.

With the pull request

* Current master has issues with the menu states, the pull request does not fix them, even introduces conditions where the menu klicks (multiple klicks) do not change expected behavior any more.

Yes, yet another bug, sorrry. Thank you for reporting it.

* Having multi language subtitles using NEXTSUBTITLE to select a forced subtitle, does not show the forced subtitle (this does not happen when selecting the first language or default language forced subtitle).

Yes, that's it. It's another edge case I missed. I've fixed it now and will push the corrected version soon.

* The toggle does not change the currently selected forced or non-forced subtitle in the menu, which makes menu interactions ambiguous.

Ah, but this is intended. That behaviour is as expected and is the whole point of these changes: the automatic display of forced subtitles is achieved without loosing track of the user's track selection. It has to be so, otherwise there would be no way to know which track to toggle back to. Not being able to toggle back is the problem I am fixing.

  Upon the toggle to disable a subtitle, the OSD can show "subtitle: off", but should show the forced subtitle selected in the menu and vice-versa.

No it shouldn't. The forced subtitle is being shown via the forcing mechanism and not by being selected. The menu shows the user's original choice of subtitle because that is what is still selected in the state. Because of my changes the forcing mechanism doesn't need to mess around with the users choice.

* When selecting a forced subtitle, the two menu "enable/disable non-forced/forced subtitles" are incorrect/confusing, as you are treating it as a non-forced subtitle.

Yes, I agree that things are a little confusing when the user explcitly chooses a forced subtitle as their main subtitle choice. This does at least now work as Peter requested. Later we might consider removing the forced subtitles from the list so that this confusing state cannot be reached. We could have a second list of forced subtitles. But it's good enough at it is. Because of the forcing mechanism, users never need to explicitly choose the forced subtitle (they will see it automatically when subtitles are off via the forcing mechanism).

• After a select, do you evaluate the type of the subtitle?

Yes. A user selection now performs two operations. It sets the current main subtitle to the one the user chooses, and it also searches for a language matching forced subtitle and stores that for use when forcing.

• Are you using the two toggles to control behavior in your logic? Or is it, that you have your logic and are setting the values for these toggles?

The former. I do not put any code in the way of the user's control of these states. All my changes are to do with how those states behave.

In the mailing list I proposed to simplify things (e.g. reducing to have one menu subtitle toggle on/off, unfortunately nobody commented). I think the two on/off states and the toggle states collide and are difficult to handle with menu and key events, and expose them unambiguous back to the menu.

That has been discussed. There are attractions in the simplicity, but it means losing the secondary forcing mechanism for forced subtitles. It would make it impossible to toggle with a single button press between the two most useful states, those being "Having the movie showing you subtitles for the cases where it's been decided the user will very likely need them" and "Displaying the subtitle track the user has chosen to explicitly enable."

The two toggles are absolutely necessary because there are two mechanisms via which subtitles are displayed: via forcing and via user selection.

There is no problem reflecting the states in the menus. It's just looks that way because of confusion due partly because of bugs and partly if not understanding that forcing is a separate mechanism that doesn't involve updating the user's selection.

Do you want to fix this in this pull request?

I've lost track of exactly what this is referring to. I think probably no. I've hoping that the bug fix I'm about to push is will be the end of it.

<style> </style>

Having Event Actions

only forced toggle ON selects forced
OSD: "forced: on"
Menu: select forced (already selected)
menu: "disable subtitle"

  toggle OFF 	OSD: "forced: off"
    	menu: "enable subtitle"
    	 
  subtitle ON (menu) 	same toggle
    	 
  subtitle OFF (menu) 	same toggle

only non-forced toggle ON selects non-forced
OSD: "non-forced: on"
Menu: select forced (already selected)
menu: "disable subtitle"

  toggle OFF 	OSD: "non-forced: off"
    	menu: "enable subtitle"
    	 
  subtitle ON (menu) 	same toggle
    	 
  subtitle OFF (menu) 	same toggle

Both showing forced
toggle ON selects non-forced
OSD: "non-forced: on"
Menu: select non-forced

  showing non-forced 	 
  toggle OFF 	selects forced
    	OSD: "forced: on" or "non-forced: off"
    	Menu: select forced
    	 
  subtitle ON (menu) 	enable show subtitle (forced or non-forced)
    	OSD: "non-forced: on"
    	menu: "disable subtitle"
    	 
    	OR (when off means forced always shown):
    	so forced was shown
    	selects non-forced subtitle
    	OSD: "non-forced: on"
    	Menu: select non-forced
    	menu: "disable subtitle"
    	 
  subtitle OFF (menu) 	disable show subtitle (forced or non-forced)
    	OSD: "forced: off" or "non-forced: off"
    	menu: "enable subtitle"
    	 
    	OR (when off means forced always shown):
    	so non-forced was shown
    	selects forced subtitle
    	OSD: "forced: on" or "non-forced: off"
    	Menu: select forced
    	menu: "enable subtitle"
    	 
    	I think you prefer the "OR" behavior.
    	When having both subtitles, this is more inline with the toggle experience, and would also be my preferred.

Would this also be your expected behavior?

Sorry, I'm finding that very difficult to follow. I'm not even quite sure what the Having column is. One thing to be aware of is that the intention is that the user never needs to explicitly select a forced subtitle. The interface is allowing that, but it's not really a useful state.

The new version that I recently pushed fixes the problem that led to explcit selection and enabling of a forced subtitle not always causing that subtitle to be shown. Here is a table showing the intended behaviour - what I see when I test, irrespective of how I get to these states:

The user also has the option to disable forced subtitles in which case the behaviour (differing only in the 3rd column of the 2nd line) is:

I haven't looked into your updated pull request, but commenting on your last two comments.

In my last check I noticed that with multiple languages and accessing the menu the results are unpredictable. Also it needs multiple different presses to come again into a desired mode. The OSD does not tell the truth; have seen cases were it says ON, but actually nothing was shown.

Do you want to fix this in this pull request?

I've lost track of exactly what this is referring to. I think probably no. I've hoping that the bug fix I'm about to push is will be the end of it.

I am referring to simplifying the subtitle menu and coming into a code state were the mentioned unexpected behaviors are solvable.

Subtitle menu selections should address your "to toggle with a single button press between the two most useful states". I don't see that this wouldn't be possible, after removing one of the two subtitle ON/OFF toggles, because that is interfering with your logic.

I haven't looked into your updated pull request, but commenting on your last two comments.

In my last check I noticed that with multiple languages and accessing the menu the results are unpredictable. Also it needs multiple different presses to come again into a desired mode. The OSD does not tell the truth; have seen cases were it says ON, but actually nothing was shown.

In your last check, yes. That was the bug. It was unpredictable, I know exactly why and I have fixed the problem.

Do you want to fix this in this pull request?

I've lost track of exactly what this is referring to. I think probably no. I've hoping that the bug fix I'm about to push is will be the end of it.

I am referring to simplifying the subtitle menu and coming into a code state were the mentioned unexpected behaviors are solvable.

Other than the possibility of remaining bugs, which of course I cannot guarantee, there are now no unexpected behaviours (unexpected to me at least). The only behaviour that some might consider unexpected is having English (say) selected, having subtitles Off and seeing English (forced) subtitles displayed, but that is the whole intent in supporting forcing of subtitles.

Subtitle menu selections should address your "to toggle with a single button press between the two most useful states". I don't see that this wouldn't be possible, after removing one of the two subtitle ON/OFF toggles, because that is interfering with your logic.

That is possible, but some users may at times wish to have no subtitles displayed at all (even the forced ones). Without the "Forced subtitle On/Off" toggle, there would be no way for a user to choose that behaviour. Another way to explain it is that the "Forced subtitle On/Off" toggle allows the user to choose between the two tables I present in my previous comment.

I realise the multiple bugs in earlier versions have made testing this difficult, but the tables I show above have always been the intended behaviour. It has just taken some time to get there.

That is possible, but some users may at times wish to have no subtitles displayed at all (even the forced ones). Without the "Forced subtitle On/Off" toggle, there would be no way for a user to choose that behaviour. Another way to explain it is that the "Forced subtitle On/Off" toggle allows the user to choose between the two tables I present in my previous comment.

I thought we had at one point the consensus to always at minimum show forced subtitles.

This would have simplified interactions, by removing one of the on/off toggles, and making behavior predictable/understandable.

Giving that you want to keep "disable forced subtitle", I tested current master and your updated pull request.

It is working fine, up till a forced subtitle is selected, as it behaves as a non-forced subtitle. OSD information is incorrect and the menu toggles states are incorrect and behave unexpected. As mentioned before, I have seen the OSD informs ON, no subtitle is shown.

As you have implemented a swap mechanism, the forced/non-forced subtitle on/off logic and the selection of forced/non-forced subtitles would need to be projected into the "swap state".

That is possible, but some users may at times wish to have no subtitles displayed at all (even the forced ones). Without the "Forced subtitle On/Off" toggle, there would be no way for a user to choose that behaviour. Another way to explain it is that the "Forced subtitle On/Off" toggle allows the user to choose between the two tables I present in my previous comment.

I thought we had at one point the consensus to always at minimum show forced subtitles.

The option to turn off forced subtitles predates my changes. My preference is to keep that option, but I'm not against removing it if the devs okayed doing so. It's a little difficult to talk about "consensus" when it is just you and I discussing it.

This would have simplified interactions, by removing one of the on/off toggles, and making behavior predictable/understandable.

Once implemented correctly, the cases with forced subtitles toggled off should be the simplest and easiest to understand: the behaviour should be:

• if the main subtitle enable is on, you see the selected subtitle
• if the main subtitle enable is off, you see no subtitles

Giving that you want to keep "disable forced subtitle", I tested current master and your updated pull request.

Thank you for testing yet again. Your efforts are much appreciated.

It is working fine, up till a forced subtitle is selected, as it behaves as a non-forced subtitle. OSD information is incorrect and the menu toggles states are incorrect and behave unexpected. As mentioned before, I have seen the OSD informs ON, no subtitle is shown.

Gah! Really?! I definitely found one cause of that. I fixed it. I tested. There is the problem that I am testing fixes-33 and then taking my changes over to master without being able to test on master. I may have to shelve this until the next release, when I'll be able to work on a fixes branch that's very close to master, at least for a while.

I will test again. Please say if you can recall a particular sequence of commands that gave rise to one of these states, particularly the ON with nothing showing case - although anything not matching the tables I posted a few comments back would be useful.

As you have implemented a swap mechanism, the forced/non-forced subtitle on/off logic and the selection of forced/non-forced subtitles would need to be projected into the "swap state".

The two tables from a few comments back should explain at least the intention in how those settings project to the swap state.

A problem with the swap mechanism is that the user can select as their main subtitle the same track as the forced track. In that case swapping (unless special cased) does nothing because the two things swapped between are the same. That would lead to TOGGLECC showing a message reporting toggling of the on/off state, but the same subtitle is displayed: it would be displayed in the on state because it is selected and it would be displayed in the off state because it is forced. To avoid that weirdness, I turn off forcing when a user selects a forced subtitle. That way they should see the subtitle only in the on state. That special casing was added to address one of Peter's comments.

I've tested the fixes-33 version again and haven't been able to provoke any incorrect behaviour. I've also closely checked the match between the changes on fixes-32 and those on master and I can't see any differences. The unchanged code of fixes-33 and master differ to some degree in SubtitleReader::AddAVSubtitle, which is one of the methods altered, but the code changes look to have been made appropriately to account for that.

One test is to use NEXTSUBTITLE to step through each subtitle one by one with subtitles enabled. I see, as expected:

With subtitles disabled, I see:

That again is exactly as intended. If you are seeing different results, I have no explanation.

If you have external subtitles (srt file), the forced subtitles do not show, in fact no subtitles show until you enable them.

With the setting "Always display subtitles", the English subtitles start when play starts, and toggling turns them off. The forced subtitles do not show unless you specifically enable them.

Both of these behaviours were this way before your pull request. They are not entirely logical. They are outside the scope of this pull request. They can be fixed later if needed.

I have merged the commits into master

I tried some two weeks ago with an updated master and the pull request.

It is working fine, up till a forced subtitle is selected, as it behaves as a non-forced subtitle. OSD information is incorrect and the menu toggles states are incorrect and behave unexpected. As mentioned before, I have seen the OSD informs ON, no subtitle is shown.

I could not reproduce this one. I thought I could easily reproduce it and I do not have an explanation that I couldn't anymore, as the source hasn't changed.
The issues with the menu as described before still existed.

Peter was just half a day earlier with merging into master. I earliest had time today. Tested with merged master and only used keys:

Global subtitles ON
START movie: -> german non-forced

TOGGLECC: 'Subtitle 4: German "German" Off' -> german forced

NEXTSUBTITLE: 'Subtitle 5: French "French"' (no On/Off shown) -> french forced, expected would be to show french non-forced

NEXTSUBTITLE: 'Subtitle 6: French "French forced" (forced)' (no On/Off shown) -> no subtitle shown, expected would be to show french forced

NEXTSUBTITLE: 'Subtitle 1: Englisch "English forced" (forced)' (no On/Off shown) -> no subtitle shown, expected would be to show french forced

NEXTSUBTITLE: 'Subtitle 2: English"' (no On/Off shown) -> english forced, expected would be to show english non-forced

TOGGLECC: 'Subtitle 2: English" On' -> english non-forced

TOGGLECC: 'Subtitle 2: English" Off' -> english forced

Would be nice NEXTSUBTITLE enables the subtitle (forced or non-forced), and in the OSD it shows explicitly "On".
A similar behavior should also be used for the menu to solve the discrepancies.

Peter - thanks for getting that in.

Angela - those results are exactly as currently intended, but I clearly understand your reasoning for expecting otherwise. You have identified the distinction is whether NEXTSUBTITLE should just update the selection or whether it should also put subtitles into the enabled state. That's not something I have an opinion on and didn't wish to change it in this pull request. We'd need devs-wide agreement for that.

I do intend to make further improvements, but not until after the next mythtv release.